-
Notifications
You must be signed in to change notification settings - Fork 9.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
*: use machine default host only for default value, 0.0.0.0 #7394
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7394 +/- ##
==========================================
- Coverage 71.12% 70.83% -0.29%
==========================================
Files 236 236
Lines 21093 21093
==========================================
- Hits 15003 14942 -61
- Misses 4970 5026 +56
- Partials 1120 1125 +5
Continue to review full report at Codecov.
|
embed/config.go
Outdated
@@ -381,8 +381,8 @@ func (cfg *Config) UpdateDefaultClusterFromName(defaultInitialCluster string) { | |||
// if client-listen-url is 0.0.0.0, just use detected default host | |||
// otherwise, rewrite advertise-client-url with localhost | |||
if ip != "0.0.0.0" { | |||
_, acPort, _ := net.SplitHostPort(cfg.ACUrls[0].Host) | |||
cfg.ACUrls[0] = url.URL{Scheme: cfg.ACUrls[0].Scheme, Host: fmt.Sprintf("localhost:%s", acPort)} | |||
ahost, acPort, _ := net.SplitHostPort(cfg.ACUrls[0].Host) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong. Why do you parse the host and port and set the exactly same host and port back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR (https://github.com/coreos/etcd/pull/7027/files) does the wrong thing. We should not blindly rewrite the advertise url to localhost just because the ip is not 0.0.0.0. In another word, even if cfg.IsDefaultHost() returns true, it does not mean that we have overwritten both client and peer advertise urls. That is why we might overwrite the users configuration.
Please make sure you fully understand what happened before making the change.
eb977f4
to
a8b8cd9
Compare
f681373
to
7ebb72b
Compare
7ebb72b
to
cf5e537
Compare
@heyitsanthony @xiang90 PTAL. Now |
This seems complicated/invasive for what it does and it doesn't fix the original issue since it doesn't affect the yaml path. I think the fix is to keep DefaultAdvertise as |
cf5e537
to
73f3e0a
Compare
73f3e0a
to
c21f4c5
Compare
@heyitsanthony @xiang90 PTAL. linux
darwin
|
embed/config.go
Outdated
|
||
used := false | ||
pip, pport, _ := net.SplitHostPort(cfg.LPUrls[0].Host) | ||
if cfg.defaultPeerHost() || pip == "0.0.0.0" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&&
? no given advertise peer and the listen address is 0.0.0.0 => use default host, right?
embed/config.go
Outdated
used = true | ||
} | ||
cip, cport, _ := net.SplitHostPort(cfg.LCUrls[0].Host) | ||
if cfg.defaultClientHost() || cip == "0.0.0.0" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&&
?
embed/config.go
Outdated
cfg.ACUrls[0] = url.URL{Scheme: cfg.ACUrls[0].Scheme, Host: fmt.Sprintf("localhost:%s", acPort)} | ||
cfg.InitialCluster = cfg.InitialClusterFromName(cfg.Name) | ||
} | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no naked returns
embed/config.go
Outdated
} | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no naked returns
func (cfg *Config) UpdateDefaultClusterFromName(defaultInitialCluster string) { | ||
defaultHost, defaultHostErr := cfg.IsDefaultHost() | ||
defaultHostOverride := defaultHost == "" || defaultHostErr == nil | ||
if (defaultHostOverride || cfg.Name != DefaultName) && cfg.InitialCluster == defaultInitialCluster { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happened to cfg.Name != DefaultName) && cfg.InitialCluster == defaultInitialCluster
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misread cfg.InitialCluster
as AdvertisePeerURL somehow. Now fixed. PTAL. Thanks!
e42a33e
to
7f12c74
Compare
7f12c74
to
5cd442a
Compare
5cd442a
to
fda9ad3
Compare
sgtm, defer to @xiang90 about behavior for |
will this need a backport? |
@gyuho Can we add a unit test to cover the bad case? |
If user only gives
, we still want etcd to start. |
@gyuho Probably we should document the use case I mentioned above? |
fda9ad3
to
6424443
Compare
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
6424443
to
4aa68e0
Compare
@xiang90 @heyitsanthony Added test cases PTAL. |
LGTM |
@gyuho merge this? |
Fix #7348.
#7027 broke this.
It overwriting the advertise-client-urls' host with 'localhost'.
Signed-off-by: Gyu-Ho Lee gyuhox@gmail.com
Contributing guidelines
Please read our contribution workflow before submitting a pull request.